Fix bug 64823: ZTS GD fails to to find system TrueType font #17366
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
First, the `$fontfile` parameter actually supports a semicolon delimited list of fonts (as documented[1]); thus passing the full string to `VCWD_REALPATH()` or `php_check_open_basedir()` makes no sense; we could pass the individual parts, but … Second, libgd uses an elaborate font detection. There is a hard- coded `DEFAULT_PATH` which can be overridden by the environment variable `GDFONTPATH`. Semantics are like the `PATH` environment variable. If `DEFAULT_PATH` was still exposed (it is no longer as of libgd 2.1.0[2]), we could take that into account, but … External libgd can be configured with font-config support, so font aliases and even lookup patterns are supported. There is no way to cater to that upfront. Thus, we no longer interfere with libgd's font lookup. Checking the realpath was already doubtful (we didn't even use the resolved path). Lifting the open_basedir restriction is a bit more delicate, but the manual still states that open_basedir would not apply, and more relevant, not much harm can be done, because libgd only passes the found font to `FT_New_Face()` which likely fails for any non font files without any error which could reveal sensitive information. And the font file is never written. It should be noted that this solves lookup of system fonts, does not change the behavior for absolute font paths, but still does not resolve issues with relative paths to font files in ZTS environments using external libgd (our bundled libgd has a workaround for that). This particular issue cannot be solved, so users of ZTS builds still need to add `realpath(.)` to the `GDFONTPATH` as documented in the manual (or pass absolute paths as `$fontfile`). [1] <https://www.php.net/imagettftext> [2] <libgd/libgd@2a921c8>
|
Hmm, maybe use Ugh, we're using bundled libgd for macOS CI, but this lacks the proper |
|
In the meantime I've learned that
So I don't think a test makes much sense. We would either need to know where the font is stored on the executing system and set |
devnexen
approved these changes
Jan 6, 2025
|
I'll leave this open for a while, since removing the open_basedir check might be objectionable. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
First, the
$fontfileparameter actually supports a semicolon delimited list of fonts (as documented[1]); thus passing the full string toVCWD_REALPATH()orphp_check_open_basedir()makes no sense; we could pass the individual parts, but …Second, libgd uses an elaborate font detection. There is a hard- coded
DEFAULT_PATHDEFAULT_FONTPATHwhich can be overridden by the environment variableGDFONTPATH. Semantics are like thePATHenvironment variable. IfDEFAULT_PATHDEFAULT_FONTPATHwas still exposed (it is no longer as of libgd 2.1.0[2]), we could take that into account, but …External libgd can be configured with font-config support, so font aliases and even lookup patterns are supported. There is no way to cater to that upfront.
Thus, we no longer interfere with libgd's font lookup. Checking the realpath was already doubtful (we didn't even use the resolved path). Lifting the open_basedir restriction is a bit more delicate, but the manual still states that open_basedir would not apply, and more relevant, not much harm can be done, because libgd only passes the found font to
FT_New_Face()which likely fails for any non font files without any error which could reveal sensitive information. And the font file is never written.It should be noted that this solves lookup of system fonts, does not change the behavior for absolute font paths, but still does not resolve issues with relative paths to font files in ZTS environments using external libgd (our bundled libgd has a workaround for that). This particular issue cannot be solved, so users of ZTS builds still need to add
realpath(.)to theGDFONTPATHas documented in the manual (or pass absolute paths as$fontfile).[1] https://www.php.net/imagettftext
[2] libgd/libgd@2a921c8
Targeting master due to the removal of the open_basedir check.